Skip to content

Comments

Improve test coverage: Add fix-permissions script and pip index-url v…#97

Open
smoparth wants to merge 1 commit intoopendatahub-io:mainfrom
smoparth:issue/88-fix-permissions-and-pip-index-url-tests
Open

Improve test coverage: Add fix-permissions script and pip index-url v…#97
smoparth wants to merge 1 commit intoopendatahub-io:mainfrom
smoparth:issue/88-fix-permissions-and-pip-index-url-tests

Conversation

@smoparth
Copy link
Member

@smoparth smoparth commented Feb 23, 2026 •
edited
Loading

Add fix-permissions script and pip index-url validation tests

  • Add file_executable() helper to ContainerRunner
  • Add test_fix_permissions_executable to verify /usr/local/bin/fix-permissions is present and executable in both Python and CUDA images
  • Add test_pip_index_url_configured to assert pip global.index-url is actually set (not just that [global] exists)

Closes #88

Description

How Has This Been Tested?

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • Tests
    • Added an executable-file check to the test runner to verify file executability.
    • Added tests to verify the presence and executability of the fix-permissions utility and to confirm pip global index URL is configured and retrievable.

@coderabbitai
Copy link

coderabbitai bot commented Feb 23, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

Adds a new ContainerRunner helper to check file executability and introduces tests that verify the fix-permissions script is executable and that pip's global.index-url is configured. Duplicate test function declarations were also introduced in the same test file.

Changes

Cohort / File(s) Summary
Test Infrastructure
tests/conftest.py		 Added file_executable(self, path: str) -> bool to ContainerRunner which runs test -x <path> to check file executability (mirrors existing file_exists behavior).
Test Cases (with duplicates)
tests/test_common.py		 Added test_fix_permissions_executable(container) to assert /usr/local/bin/fix-permissions is executable and test_pip_index_url_configured(container) to assert pip config get global.index-url returns a non-empty value. Each test appears twice (duplicate declarations present).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title mentions 'fix-permissions script and pip index-url' which are the main changes, but is truncated (appears incomplete with 'v…').
Linked Issues check ✅ Passed The PR implements both coding objectives from issue #88: adding file_executable() helper method and two test functions (test_fix_permissions_executable and test_pip_index_url_configured) that validate the fix-permissions script executability and pip index-url configuration.
Out of Scope Changes check ✅ Passed All changes directly address issue #88 requirements: ContainerRunner.file_executable() supports testing script executability, and both new tests validate the required conditions without unrelated modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot reviewed Feb 23, 2026
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/test_common.py (1)

96-103: Include result.stderr in the failure message for easier debugging.

When pip config get global.index-url exits non-zero it writes the reason to stderr, but the current assertion message is static. Including it surfaces the actual pip error without changing test semantics.

🔧 Proposed improvement 
-    assert result.returncode == 0, (
-        "pip global.index-url is not set — expected an index-url in /etc/pip.conf"
-    )
+    assert result.returncode == 0, (
+        f"pip global.index-url is not set — expected an index-url in /etc/pip.conf\n"
+        f"pip stderr: {result.stderr.strip()}"
+    )
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/test_common.py` around lines 96 - 103, The assertion in
test_pip_index_url_configured does not include result.stderr when
container.run("pip config get global.index-url") fails, making failures hard to
diagnose; update the first assert for result.returncode == 0 in the
test_pip_index_url_configured function to append or interpolate result.stderr
(and optionally result.stdout) into the failure message so the pip error output
from container.run is visible when the test fails.
🤖 Prompt for all review comments with AI agents 
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/test_common.py`:
- Around line 96-103: The assertion in test_pip_index_url_configured does not
include result.stderr when container.run("pip config get global.index-url")
fails, making failures hard to diagnose; update the first assert for
result.returncode == 0 in the test_pip_index_url_configured function to append
or interpolate result.stderr (and optionally result.stdout) into the failure
message so the pip error output from container.run is visible when the test
fails.
ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between ebdbfd2 and 9204637.

📒 Files selected for processing (2)
  • tests/conftest.py
  • tests/test_common.py

@smoparth
Improve test coverage: Add fix-permissions script and pip index-url v…
197d28d 
…alidation tests opendatahub-io#88

- Add file_executable() helper to ContainerRunner
- Add test_fix_permissions_executable to verify /usr/local/bin/fix-permissions is present and executable in both Python and CUDA images
- Add test_pip_index_url_configured to assert pip global.index-url is actually set (not just that [global] exists)

Closes opendatahub-io#88

Signed-off-by: Shanmukh Pawan <smoparth@redhat.com>
@smoparth smoparth force-pushed the issue/88-fix-permissions-and-pip-index-url-tests branch from 9204637 to 197d28d Compare February 23, 2026 22:17
shifa-khan
shifa-khan reviewed Feb 24, 2026
View reviewed changes
Copy link
Contributor

@shifa-khan shifa-khan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a suggestion, should we consider adding similar tests for uv?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shifa-khan shifa-khan shifa-khan left review comments

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@LalatenduMohanty LalatenduMohanty Awaiting requested review from LalatenduMohanty

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve test coverage: Add fix-permissions script and pip index-url validation tests

2 participants

@smoparth @shifa-khan